-
-
Notifications
You must be signed in to change notification settings - Fork 747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework the packs pack (more packs-related ChatOps!) #2982
Conversation
WIP! Like, really WIP. Don't test it yet, there's still some work to be done. |
Btw, |
Current coverage is 77.11% (diff: 100%)@@ master #2982 diff @@
==========================================
Files 430 430
Lines 21997 21997
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 16914 16962 +48
+ Misses 5083 5035 -48
Partials 0 0
|
@emedvedev Yes, I think I think with the release of Stackstorm-exchange the bitbucket rule post_receive_webhook.yaml should be removed. |
Once we make the move, that rule and workflow will become a bit simpler, too!
I'll skim through all the packs to make sure all the |
pack_formatted = None | ||
# Try to get the original yaml file because we know | ||
# how it's stored in the Exchange, otherwise fall back | ||
# to transforming json. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emedvedev
As I see, we already getting pack meta from index from endpoint: https://index.stackstorm.org/v1/index.json
And then we're downloading https://index.stackstorm.org/v1/packs/github.yaml again with the same meta.
What was initial idea here? Because it seems a bit dirty with all the hardcoding, additional request, regex.
I'm thinking about removing this second pack.yaml
part and rely only on get_pack_from_index(pack)
meta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about removing this second pack.yaml part and rely only on get_pack_from_index(pack) meta.
👍
Problem with json->yaml is that it's not ordered, but that doesn't really matter much.
- `!pack get` - get info about installed pack - `!pack show` - show remote StackStorm Exchange pack - polished & working `!pack show`
So it's fixed & Polished & Acceptance tested in Slack.
The Aliases automated tests as well as Integration Tests (e2e, robo) are in my TODO list and will be added, but It's good to merge now to see things in pre-production (packages).
|
❤️ ❤️ ❤️ ❤️ LGTM. |
I'm kinda missing hubot alias before the command, though I'm not sure we can get it now in any sane way. Also, #3051 to prevent different kinds of output formatting for different packs. Other than that, looks good to me. |
upgrade: | ||
type: "boolean" | ||
default: false | ||
description: "Upgrade the pack if already installed." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait a second. I don't see us using it in any way. I also don't think we should have something like that to begin with. Can settle to default: true
, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what that flag is doing here 😛 Should just remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know where it comes from too. Will remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, but this is interesting.
In future it would be good to have pack install really idempotent.
So if pack is already installed - it won't be reinstalled again, unless some special flag like that specified. Re-downloading/re-installing by default takes time and it's in fact "upgrade" or "reinstall".
This makes sense especially for configuration management tools, when you apply playbook/cookbook, which apart of deploying st2
installs a bunch of st2 packs on the same machine. And it's normal to run same playbook on the same machine several times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bigger discussion that applies to package management in general. It will be idempotent if you specify a precise version, and we already support that, but a repeated install
is normally regarded as upgrade, we're certainly not alone here. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Installing + Downloading over and over again (even if it reaches the same state) is not considered really "Idempotent" in configuration management.
If resources is in desired state - we shouldn't repeat the same operation again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair. They do stay in that state though 😛
Let's make sure we get to that issue after the release, it's pretty important.
Yeah, @enykeev I just verified, and there is no hubot alias variable in context. I bet none could think that it might be useful to have in context. Might be something good to adjust in future. |
Nice work everyone! |
Aliases for the packs pack:
—
pack install
for installing a pack or multiple packs (just a minor rework here);—
pack search
to query the index;—
pack show
to display information about an installed pack, including HEAD location in the git tree (N commits ahead/behind);—
pack show available
to fetch information about a pack from the index.Most of
packs.deploy
is now obsolete, so I'm removing it and the helper actions for now, but I would appreciate @jjm finding some time to talk it over with me. A thing or two that was possible withpacks.deploy
might not be covered by our new pack management, and if that's the case, I'm fine with keeping the deploy workflow or breaking it down into multiple actions.There was a
linux
pack directory and an action. Inside thepacks
pack. Apparently, for quite some time, too (I remember @enykeev noticing it). I removed it. Sorry, not sorry.Some minor cleanups (this PR started as a minor cleanup, actually, but then things just got more and more interesting).